-
Notifications
You must be signed in to change notification settings - Fork 244
refactor: simplify components.json code #7742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b43d562 to
33c1e10
Compare
33c1e10 to
2422aa9
Compare
There are no more instances of this. Signed-off-by: James Le Cuirot <[email protected]>
There are no more instances of this. Signed-off-by: James Le Cuirot <[email protected]>
2422aa9 to
09b286b
Compare
jq is very powerful, you know. :) Signed-off-by: James Le Cuirot <[email protected]>
We almost always pass `${OS}` so make this the default.
testPackagesInstalled() was not testing OS Guard properly because OS and OS_VERSION were being set to Ubuntu and 3.0, matching nothing from components.json. Several components are not installed for OS Guard, but rather than add yet more shell code to handle this, simplify things by making it possible to use the OS variant in components.json. OS Guard's VARIANT_ID is osguard, but this is upper-cased by our scripts. Use DEFAULT to match an OS without VARIANT_ID. Signed-off-by: James Le Cuirot <[email protected]>
Signed-off-by: James Le Cuirot <[email protected]>
09b286b to
ae35d90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the components.json handling to simplify the code and add support for OS variants (specifically OS Guard). The changes eliminate fallback compatibility code for deprecated versions and multiArchVersions fields, streamline jq-based JSON parsing, and enable OS variant-specific package configurations.
Changes:
- Removed fallback support for deprecated
versionsandmultiArchVersionsfields in components.json - Simplified package version and URL retrieval using more efficient jq filters
- Added OS variant support to distinguish between standard Azure Linux and Azure Linux OS Guard configurations
- Updated components.json to use
azurelinux/DEFAULTandazurelinux/OSGUARDvariants for GPU-related packages - Removed OS Guard-specific conditional logic from installation scripts by using
<SKIP>markers in components.json
Reviewed changes
Copilot reviewed 24 out of 80 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| parts/linux/cloud-init/artifacts/cse_helpers.sh | Core refactor: replaced updateRelease() with getPackageJSON(), simplified version/URL extraction with jq |
| parts/linux/cloud-init/artifacts/cse_install.sh | Added OS_VARIANT parameter to package lookup functions |
| parts/linux/cloud-init/artifacts/ubuntu/cse_install_ubuntu.sh | Pass OS_VARIANT to credential provider installation |
| parts/linux/cloud-init/artifacts/mariner/cse_install_mariner.sh | Pass OS_VARIANT to credential provider installation |
| parts/linux/cloud-init/artifacts/azlosguard/cse_install_osguard.sh | Pass OS_VARIANT to credential provider installation |
| vhdbuilder/packer/install-dependencies.sh | Removed OS Guard conditional checks for containerd, oras, and GPU packages |
| vhdbuilder/packer/test/linux-vhd-content-test.sh | Added OS Guard SKU detection and OS_VARIANT initialization |
| parts/common/components.json | Added OSGUARD variant with SKIP markers, changed 3 azurelinux entries to azurelinux/DEFAULT |
| schemas/components.cue | Added azurelinux/DEFAULT and azurelinux/OSGUARD to schema |
| pkg/vhdbuilder/datamodel/component_configs.go | Changed MultiArchVersions from string array to struct array |
| pkg/vhdbuilder/datamodel/component_configs_test.go | Updated test expectations for new multiArchVersionsV2 structure |
| e2e/components/components.go | Updated to check for DEFAULT variant when reading package versions |
| spec/parts/linux/cloud-init/artifacts/test_components.json | Removed deprecated multiArchVersions and versions test data |
| spec/parts/linux/cloud-init/artifacts/cse_helpers_spec.sh | Updated tests to use new getPackageJSON function, fixed typo |
| parts/linux/cloud-init/artifacts/README-COMPONENTS.md | Added documentation about OS variant support |
| } | ||
| }, | ||
| "azurelinux": { | ||
| "azurelinux/DEFAULT": { |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR changes only 3 instances of "azurelinux" to "azurelinux/DEFAULT" (at lines 1708, 1742, and 1776), but leaves 7 other instances unchanged. This inconsistency means that most azurelinux configurations are not explicitly marked as DEFAULT variant. This could lead to confusion about which packages support the OSGUARD variant and which don't. Consider either: (1) updating all azurelinux keys to azurelinux/DEFAULT for consistency, or (2) documenting why only these three packages (datacenter-gpu-manager-4-core, datacenter-gpu-manager-4-proprietary, dcgm-exporter) need the DEFAULT distinction.
| "azurelinux/DEFAULT": { | |
| "azurelinux": { |
| "azurelinux/DEFAULT"?: #AzureLinuxOSDistro | ||
| "azurelinux/OSGUARD"?: #AzureLinuxOSDistro | ||
| azurelinux?: #AzureLinuxOSDistro |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema defines both "azurelinux/DEFAULT" and azurelinux as valid keys in the same schema. This creates ambiguity and potential conflicts. Lines 90 and 92 define overlapping keys where azurelinux could match both patterns. Consider removing line 92 to avoid ambiguity since the code now explicitly handles the DEFAULT variant.
| containerImage=$(readContainerImage "mcr.microsoft.com/dummyImageWithOldMultiArchVersions") | ||
| When call updateMultiArchVersions "$containerImage" | ||
| The variable MULTI_ARCH_VERSIONS[@] should equal "dummyVersion3 dummyVersion4 dummyVersion5" | ||
| The variable MULTI_ARCH_VERSIONS[@] should equal "dummyVersion1.1 dummyVersion1 dummyVersion2.1 dummyVersion2" |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test on line 137 expects multi-arch versions in a different order than before ("dummyVersion1.1 dummyVersion1 dummyVersion2.1 dummyVersion2" vs the old "dummyVersion1.1 dummyVersion2.1 dummyVersion1 dummyVersion2"). This suggests the new jq logic using map(.latestVersion, .previousLatestVersion) changes the ordering. While this may not be functionally critical, it's worth documenting whether the order matters for any consumers of this data.
| Expect(imageList).To(HaveLen(3)) | ||
| Expect(imageList[0]).To(Equal("mcr.microsoft.com/unittest/mockimage:mock.000000.1")) | ||
| Expect(imageList[1]).To(Equal("mcr.microsoft.com/unittest/mockimage:mock.000000.2")) | ||
| Expect(imageList[2]).To(Equal("mcr.microsoft.com/unittest/mockimage:mock.000000.1")) |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects 3 images in the image list (line 223) but the comment on line 226 suggests this is the "previousLatestVersion" from mock.000000.1. However, looking at the mock data, mock.000000.1 is used as both an amd64OnlyVersion and as a previousLatestVersion in the multiArchVersionsV2. This creates duplicate entries in the image list. While this matches the implementation in ToImageList(), consider whether this duplication is intentional or if the test data should be restructured to avoid confusion.
| elif [ "$OS_SKU" = "AzureLinuxOSGuard" ]; then | ||
| OS=$AZURELINUX_OS_NAME | ||
| OS_VARIANT=OSGUARD | ||
| else | ||
| OS=$UBUNTU_OS_NAME | ||
| fi | ||
| PACKAGE_VERSIONS=() | ||
| updatePackageVersions "${p}" "${OS}" "${OS_VERSION}" | ||
| updatePackageVersions "${p}" "${OS}" "${OS_VERSION}" "${OS_VARIANT}" | ||
| PACKAGE_DOWNLOAD_URL="" | ||
| updatePackageDownloadURL "${p}" "${OS}" "${OS_VERSION}" | ||
| updatePackageDownloadURL "${p}" "${OS}" "${OS_VERSION}" "${OS_VARIANT}" |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable OS_VARIANT is set only for the AzureLinuxOSGuard case (line 209) but is used unconditionally in function calls on lines 214 and 216. For other OS types (Ubuntu, Mariner, standard AzureLinux), OS_VARIANT will be undefined, which could cause unexpected behavior in the shell functions. Consider initializing OS_VARIANT="" before the OS_SKU conditionals to ensure it always has a defined value.
| # By default, use the first match from these: | ||
| # .downloadURIs.${osLowerCase}/${osVariant}.current | ||
| # .downloadURIs.${osLowerCase}.current | ||
| # .downloadURIs.default.current | ||
| local search=".downloadURIs.\"${osLowerCase}/${osVariant}\".current // .downloadURIs.${osLowerCase}.current // .downloadURIs.default.current" | ||
|
|
||
| # For AZURELINUX, check the OS version (e.g. 3.0) prefixed with "v" before "current" (e.g. v3.0). | ||
| if isMarinerOrAzureLinux "${os}"; then | ||
| if [ "$(echo "${package}" | jq -r ".downloadURIs.${osLowerCase}.\"v${osVersion}\"" 2>/dev/null)" != "null" ]; then | ||
| RELEASE="\"v${osVersion}\"" | ||
| fi | ||
| return 0 | ||
| fi | ||
| local osVersionWithoutDot=$(echo "${osVersion}" | sed 's/\.//g') | ||
| if [ "$(echo "${package}" | jq -r ".downloadURIs.ubuntu.r${osVersionWithoutDot}" 2>/dev/null)" != "null" ]; then | ||
| RELEASE="\"r${osVersionWithoutDot}\"" | ||
| search=".downloadURIs.\"${osLowerCase}/${osVariant}\".\"v${osVersion}\" // .downloadURIs.${osLowerCase}.\"v${osVersion}\" // ${search}" | ||
| # For UBUNTU, check the OS version (e.g. 20.04) with no dots and prefixed with "r" before "current" (e.g. r2004). | ||
| elif isUbuntu "${os}"; then | ||
| search=".downloadURIs.\"${osLowerCase}/${osVariant}\".\"r${osVersion//.}\" // .downloadURIs.${osLowerCase}.\"r${osVersion//.}\" // ${search}" | ||
| fi |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getPackageJSON function uses compound jq filter with the // operator for fallback logic. While this is elegant, the complex chained filter on line 868 is difficult to read and maintain. Consider adding inline comments explaining the precedence order, especially since this is a critical function that determines which package configuration is used.
| # .downloadURIs.${osLowerCase}/${osVariant}.current | ||
| # .downloadURIs.${osLowerCase}.current | ||
| # .downloadURIs.default.current | ||
| local search=".downloadURIs.\"${osLowerCase}/${osVariant}\".current // .downloadURIs.${osLowerCase}.current // .downloadURIs.default.current" |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter expansion ${os_variant} is used in the search string without quotes. If os_variant is empty or contains spaces, this could lead to unexpected jq filter construction. The filter should handle empty variant values gracefully. Consider documenting the expected behavior when osVariant is empty.
| local componentName="$2" | ||
| local os="$3" | ||
| local os_version="$4" | ||
| local os_variant=${5:-DEFAULT} |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new getLatestPkgVersionFromK8sVersion function parameter os_variant defaults to "DEFAULT" on line 901, but this is inconsistent with other functions where empty string is used. For example, in getPackageJSON, an empty osVariant is expected. This inconsistency could cause issues with the fallback logic in getPackageJSON which checks for ${osLowerCase}/${osVariant} first. Recommend using an empty string as the default to maintain consistency.
| local os_variant=${5:-DEFAULT} | |
| local os_variant="${5:-}" |
| Expect(components.ContainerImages[1].DownloadURL).To(Equal("mcr.microsoft.com/unittest/mockimage2:*")) | ||
| Expect(components.ContainerImages[1].Amd64OnlyVersions).To(HaveLen(0)) | ||
| Expect(components.ContainerImages[1].MultiArchVersions).To(HaveLen(1)) | ||
| Expect(components.ContainerImages[1].MultiArchVersions[0]).To(Equal("mock.000000.2")) | ||
| Expect(components.ContainerImages[1].MultiArchVersions[0].LatestVersion).To(Equal("mock.000000.2")) | ||
| Expect(components.ContainerImages[0].MultiArchVersions[0].PreviousLatestVersion).To(BeEmpty()) |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expectation on line 205 checks MultiArchVersions[0].PreviousLatestVersion but the index [0] refers to the first container image, not the first element being asserted. This appears to be a copy-paste error - it should check MultiArchVersions[1].PreviousLatestVersion to match the pattern from line 204 which correctly checks ContainerImages[1].MultiArchVersions[0].
| // Check if versionsV2 exists. Assume the DEFAULT OS variant. | ||
| versions := packageItem.Get(fmt.Sprintf("%s/DEFAULT.%s.versionsV2", distro, release)).Array() | ||
| if len(versions) == 0 { | ||
| versions = packageItem.Get(fmt.Sprintf("%s.%s.versionsV2", distro, release)).Array() | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The e2e test code assumes DEFAULT variant when checking package versions (line 47), but there's no fallback to handle cases where neither distro/DEFAULT nor distro paths exist. If a package is defined with only a specific variant (e.g., only azurelinux/OSGUARD), this function would return an empty array for the DEFAULT variant. Consider adding error handling or documentation about expected behavior when the DEFAULT variant is not defined.
| // Check if versionsV2 exists. Assume the DEFAULT OS variant. | |
| versions := packageItem.Get(fmt.Sprintf("%s/DEFAULT.%s.versionsV2", distro, release)).Array() | |
| if len(versions) == 0 { | |
| versions = packageItem.Get(fmt.Sprintf("%s.%s.versionsV2", distro, release)).Array() | |
| } | |
| // Check if versionsV2 exists. Prefer the DEFAULT OS variant, then non-variant, then any variant. | |
| versions := packageItem.Get(fmt.Sprintf("%s/DEFAULT.%s.versionsV2", distro, release)).Array() | |
| if len(versions) == 0 { | |
| versions = packageItem.Get(fmt.Sprintf("%s.%s.versionsV2", distro, release)).Array() | |
| } | |
| if len(versions) == 0 { | |
| // Fallback: handle cases where only specific OS variants (e.g., azurelinux/OSGUARD) are defined. | |
| versions = packageItem.Get(fmt.Sprintf("%s/*.%s.versionsV2", distro, release)).Array() | |
| } |
What this PR does / why we need it:
Firstly, this drops compatibility fallbacks for
versionsandmultiArchVersionsin components.json. There are no more instances of these.Secondly, this massively simplifies the associated code involving
jq. It's a powerful tool. 🙂Lastly, this allow the OS variant to be used in components.json for OS Guard.
testPackagesInstalled()was not testing OS Guard properly becauseOSandOS_VERSIONwere being set toUbuntuand3.0, matching nothing from components.json.Several components are not installed for OS Guard, but rather than add yet more shell code to handle this, simplify things by making it possible to use the OS variant in components.json. OS Guard's
VARIANT_IDisosguard, but this is upper-cased by our scripts. UseDEFAULTto match an OS withoutVARIANT_ID.